add missing include field for listOptions #339
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
uturunku1
commented
Mar 1, 2022
oauth_client.go
Outdated
| // A list of relations to include | ||
| type OAuthClientIncludeOp string | ||
|
|
||
| const OauthClientOauthTokens OAuthClientIncludeOp = "oauth_tokens" |
There was a problem hiding this comment.
Besides "OAuthToken" another relation listed for type OAuthClient struct is "organization". However, I did not included a constant for "organization" because I did not see it listed in atlas's allowed_values_for_include
|
I will squash these commits after PR approval |
uturunku1
force-pushed
the
update-include-field
branch
from
8aad609
to
e7999f0
Compare
|
I have two comments:
|
|
uturunku1
force-pushed
the
update-include-field
branch
from
e7999f0
to
430d9d1
Compare
sebasslash
requested changes
Mar 1, 2022
run_task.go
Outdated
| } | ||
|
|
||
| // RunTaskReadOptions represents the set of options for reading a run task | ||
| type RunTaskReadOptions struct { | ||
| // Optional: A list of relations to include with a run task. See available resources: | ||
| // https://www.terraform.io/cloud-docs/api-docs/run-tasks#list-run-tasks | ||
| Include []RunTaskIncludeOps `url:"include"` | ||
| Include []RunTaskIncludeOpt `url:"include"` |
There was a problem hiding this comment.
Ooo a missing omitempty ? 🤔
There was a problem hiding this comment.
lol, I don't apply the practices I preach. Making that change now!
uturunku1
force-pushed
the
update-include-field
branch
from
430d9d1
to
d564592
Compare
uturunku1
added a commit
that referenced
this pull request
Mar 3, 2022
revert two changes made on PR #339
Merged
uturunku1
added a commit
that referenced
this pull request
Mar 3, 2022
revert two changes made on PR #339
uturunku1
added a commit
that referenced
this pull request
Mar 4, 2022
revert two changes made on PR #339
uturunku1
added a commit
that referenced
this pull request
Mar 4, 2022
revert two changes made on PR #339
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add "include" field to listOptions structs for API that allow for include values.
These are the APIs and their include values listed in Atlas:
agent pool API: "workspaces"
oauth client API: "oauth_tokens"
policy check API: "run.workspace", "run"
run trigger API: "workspace", "sourceable"
organization API: "entitlement_set"
policy set versions API: "policy_set"
policy API: "policy_sets"
I did not implemented the changes for:
organizations API - because the relation for entitlement set was not part of
type Organization struct.policy set version API - I did have the relation present but I did not have the listOptions or listing function present.
Policy API - Policies controller has the include value "policy_sets", but this value does not align with only relation listed under
type Policy struct:Organization *Organizationjsonapi:"relation,organization"`` What am I missing here?All the other include fields were added. This is how the implementation was made:
Step 1 - Add
includestruct field to SomeResourceListOptionsStep 2 - Create constants for the allowed include values that are typed strings
Step 3 - Add a test to make sure what are getting the "included" data
Bonus Step:
Change the pre-existing typed string named
SomePrefixIncludeOpstoSomePrefixIncludeOp, liketype OAuthClientIncludeOps stringbecomestype OAuthClientIncludeOp string. I should have made the suffix singular from the beginning, but it is not too late to make that correction.To reduce noise coming from this change, you can focus on the changes made to:
Testing plan
External links
Output from tests (HashiCorp employees only)